sdk%fix: fix incorrect serialisation constants (max script size, P2P msg len), fix MnListDiffPayload serialisation routines#3
Conversation
📝 WalkthroughWalkthroughThis PR reorganizes byte accessor APIs across the codebase, updates P2P payload limits and masternode list versioning logic, increases the script decoder size limit, renames a uint accessor method, and configures feature-gating for upstream dependencies. The changes span multiple Rust packages and their tests. ChangesByte Accessor API Rename
P2P Payload Limits and Masternode List Versioning
Script Decoder Size Limit Update
Uint Accessor Method Rename
Num Feature Configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkgs/p2p_core/src/primitives/mn_list.rs (1)
113-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard Evo/version invariants before serializing
SimplifiedMnListEntry.At Line 113, serialization can silently produce malformed bytes when
mn_type == MnType::Evobutplatform_http_portorplatform_node_idisNone. Also,version < 2with non-regularmn_typecan produce non-roundtrippable output. Please enforce these invariants before writing bytes.💡 Minimal fail-fast guard (example)
pub(crate) fn encode(&self, buf: &mut Vec<u8>) { + assert!( + self.version >= 2 || self.mn_type == MnType::Regular, + "mn_type requires version >= 2" + ); + buf.extend_from_slice(&self.version.to_le_bytes()); buf.extend_from_slice(&self.pro_reg_tx_hash.to_bytes()); buf.extend_from_slice(&self.confirmed_hash.to_bytes()); buf.extend_from_slice(&self.service.addr); buf.extend_from_slice(&self.service.port.to_be_bytes()); buf.extend_from_slice(&self.operator_key.0); buf.extend_from_slice(&self.voting_key_id.0); buf.push(u8::from(self.is_valid)); if self.version >= 2 { buf.extend_from_slice(&self.mn_type.to_u16().to_le_bytes()); } if self.mn_type == MnType::Evo { - if let Some(port) = self.platform_http_port { - buf.extend_from_slice(&port.to_le_bytes()); - } - if let Some(ref nid) = self.platform_node_id { - buf.extend_from_slice(&nid.0); - } + let port = self.platform_http_port.expect("Evo entry missing platform_http_port"); + let nid = self.platform_node_id.as_ref().expect("Evo entry missing platform_node_id"); + buf.extend_from_slice(&port.to_le_bytes()); + buf.extend_from_slice(&nid.0); } }As per coding guidelines, "Use newtypes over primitives when semantics differ, enums over booleans, and make invalid states unrepresentable".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkgs/p2p_core/src/primitives/mn_list.rs` around lines 113 - 119, Before emitting bytes in the SimplifiedMnListEntry serialization path, validate the invariants: if self.mn_type == MnType::Evo then ensure self.platform_http_port.is_some() and self.platform_node_id.is_some() and return an error (or panic) if missing; likewise if self.version < 2 then ensure self.mn_type == MnType::Regular (or return an error) so non-regular types aren't serialized for older versions. Add these checks at the start of the serialization routine (the impl that writes buf for SimplifiedMnListEntry) and fail fast with a clear error message mentioning the field(s) (platform_http_port, platform_node_id, mn_type, version) so malformed/ non-roundtrippable bytes are never produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkgs/p2p_core/src/primitives/mn_list.rs`:
- Around line 113-119: Before emitting bytes in the SimplifiedMnListEntry
serialization path, validate the invariants: if self.mn_type == MnType::Evo then
ensure self.platform_http_port.is_some() and self.platform_node_id.is_some() and
return an error (or panic) if missing; likewise if self.version < 2 then ensure
self.mn_type == MnType::Regular (or return an error) so non-regular types aren't
serialized for older versions. Add these checks at the start of the
serialization routine (the impl that writes buf for SimplifiedMnListEntry) and
fail fast with a clear error message mentioning the field(s)
(platform_http_port, platform_node_id, mn_type, version) so malformed/
non-roundtrippable bytes are never produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06e6fb7d-47b2-4a3e-b53e-14a2e2936bf5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
contrib/lint/lint_javascript.pydocs/guide_rust.mdpkgs/num/Cargo.tomlpkgs/p2p_core/Cargo.tomlpkgs/p2p_core/corpus/mnlistdiff.json5pkgs/p2p_core/src/encode.rspkgs/p2p_core/src/primitives/governance.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/p2p_core/tests/mnlistdiff.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/script.rspkgs/script/src/key_id.rspkgs/script/src/lib.rspkgs/types/src/hex.rspkgs/types/src/uint.rs
Additional Information
Depends on sdk%refac: switch from
hextohex-conservative#2The
Scriptdecoder applied the interpreter limit ofMAX_SCRIPT_SIZEinstead of the serialization limit ofMAX_SIZEwhich meant otherwise legal (though unspendable) scripts caused errors when being parsed. This has been fixed.MAX_P2P_PAYLOADhas been updated to match the limits set by Dash Core withMAX_PROTOCOL_MESSAGE_LENGTH, there's little benefit to being more permissive than the reference implementation.MAX_GOV_DATAhas been dropped and restricted againstMAX_P2P_PAYLOAD, the practical limit that it'll press against as governance messages propagate over the P2P system.MnListDiffPayloaderrantly assumed that the commitments in aMnListDiffare serialized within an extra payload envelope. This has been resolved alongside a version decode bug that was discovered when a KAT vector was committed and tested against.Breaking Changes
dash-type'sPlatformNodeId,BlsPublicKeyBytes,BlsSignatureBytes,EcdsaPublicKeyBytesandEcdsaSignatureBytesanddash-script'sKeyId's{as,to}_byte_array()are now{as,to}_bytes().dash-p2p-core'sFilterTypeandProtocolVersion'sto_inner()is nowvalue().How Has This Been Tested?
cargo test --features full,_internal cargo clippy --features full,_internal --tests cargo fmt --checkChecklist